Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate configMerge and scaleMerge helpers #6022

Merged

Conversation

simonbrunel
Copy link
Member

These methods shouldn't have been public since they are specific to the chart controller internal logic. Note that this scale custom merging will be removed in v3.

Closes #5995 (private methods don't need jsdoc).

These methods shouldn't have been public since they are specific to the chart controller internal logic. Note that this scale custom merging will be removed in v3.
@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 28, 2019
benmccann
benmccann previously approved these changes Jan 28, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it may be helpful to add the jsdocs. I agree they're not required. I find them helpful sometimes as a newcomer to the codebase especially in an loosely typed language like ecmascript

etimberg
etimberg previously approved these changes Jan 28, 2019
@simonbrunel simonbrunel dismissed stale reviews from etimberg and benmccann via 1d4b41a January 29, 2019 09:14
@simonbrunel
Copy link
Member Author

I removed the explicit clone of the first argument since it's already done by helper.merge() when passing an empty object as the target. It should be a bit more optimized and I feel it makes easier understanding the intent of these methods. Added an additional test to make sure we are not modifying the defaults when merging (the reason of the previous explicit clone).

@benmccann I also added short header comments which I hope help to understand why these methods. I would not spend more effort commenting this code since it should disappear in v3 and simply replaced by helper.merge().

kurkle
kurkle previously approved these changes Jan 29, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of bytes can be saved, nothing else pops out.

src/core/core.controller.js Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
kurkle
kurkle previously approved these changes Jan 29, 2019
etimberg
etimberg previously approved these changes Jan 29, 2019
@simonbrunel simonbrunel dismissed stale reviews from etimberg and kurkle via 104991b January 29, 2019 15:26
@simonbrunel simonbrunel merged commit 0697d0d into chartjs:master Jan 29, 2019
@simonbrunel simonbrunel deleted the chore/deprecate-config-merge-helpers branch January 29, 2019 16:52
@Lekishor
Copy link

Lekishor commented Apr 1, 2019

Hi, quick question from a rather new user of chartjs.

Context:
I created some code around chartjs so that I could easily / quickly create any kind of graph with basic options already set for most of them.

In order to do so I currently use the "configMerge" helper so that I can merge a default conf object with a user defined object prior to creating the chart (using the merged result as its "options" param).

I understand why you say these are supposed to be private because they are "specific to the chart controller internal logic" but I thought it was a good thing they were exposed as helpers for cases like mine because I'm actually counting on the fact it has the same logic as the controller so that the conf passed to the chart constructor remain properly defined.

Question:
So, I was wondering if it had to really be private in the end? For me there is a purpose to keep those exposed. I still have a couple of releases to change things on my side if need be, it just made me think I might not be the only one using those helpers that way.

Thanks.

@benmccann
Copy link
Contributor

@Lekishor can you just use helpers.merge for that?

@Lekishor
Copy link

Lekishor commented Apr 2, 2019

@benmccann I could use helpers.merge sure, but as my user conf and default conf might contain options such as scale etc. it was "easier" to use the existing merge methods handling those options than re-writing something that's why for the time being I use configMerge

@simonbrunel
Copy link
Member Author

I created some code around chartjs so that I could easily / quickly create any kind of graph with basic options already set for most of them.

I'm not sure why you need to rely on those methods, can share your code?

So, I was wondering if it had to really be private in the end.

These methods are still public but deprecated. They exist only because of a wrong design of the scale options and we don't want to provide support for using them externally. The deprecation is to discourage users to rely on those methods. In v3, scales options will be changed to an object instead of arrays and this logic will be completely removed.

We are not going to un-deprecate these 2 methods but you can safely continue to use them until v3.

@Lekishor
Copy link

Lekishor commented Apr 2, 2019

@simonbrunel If scales is going to change and work as any other option in V3 then ok I won't need any other method that helpers.merge so that answer my question thanks.

About my code, I made a simple factory allowing me to create a chartjs graph and some surrounding HTML objects with a single call to a create method instead of rewriting everything every time.
I mostly need the same options for a given graph type so I predefined some default options objects and all I do is tell my factory create method what options I need (zoom, scales etc.) and from there it uses the objects I defined to create a proper config such as this :

{
      zoom:
      {
          enabled: true,
          drag: false,
          mode: 'x'
      },
      scales:
      {
            xAxes: [{
                  type: "time",
            }],
      },
}

But I also give the opportunity for a user to send its own options object to alter the default options, so a simple example would be :

{
    scales:
    {
        xAxes: [{
              stacked: true,
        }],
    }
}

In the end I just want a single objects with all my keys merged properly to send to the Chart constructor and that was what I used helpers.mergeConfig for as I was sure it would handle the merge properly for each existing options even scales.

@simonbrunel
Copy link
Member Author

@Lekishor why don't you use Chart.defaults.* instead?

@Lekishor
Copy link

Lekishor commented Apr 3, 2019

@simonbrunel Hum, to be honest I did not thought about using Chart.defaults.* for this.
In my mind this was to be modified once if you had a "fixed" predefined config you wanted to use every time you draw a specific type of graph and not something to change every time you create a new graph.

What I call my own default conf is something that is in fact modular and will change for almost each call made to my factory depending on the "options" requested as param (scales, zoom etc.) therefore it was more something to complete the user defined conf before creating the chart rather than being the actual setting of Chart.defaults. That way, the original Chart.defaults options were still there and merged with my own in the end. I wanted to merge things not replace the original default options so I coded this in order to alter the user conf passed to the graph instead.

Anyway, I think I'll go back to read the doc I might have missed something and give it a try at some point before the release of the V3 and see what happens.

Thank you for the answers / suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants